CCM-14200 - event driven callback delivery by mjewildnhs · Pull Request #33 · NHSDigital/nhs-notify-client-callbacks

mjewildnhs · 2026-02-17T17:08:02Z

Description

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I am familiar with the contributing guidelines
I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming
If I have used the 'skip-trivy-package' label I have done so responsibly and in the knowledge that this is being fixed as part of a separate ticket/PR.

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

…ds which aren't needed

mjewildnhs · 2026-02-18T13:21:26Z

@copilot review

Copilot · 2026-02-18T13:21:36Z

@mjewildnhs I've opened a new pull request, #34, to work on those changes. Once the pull request is ready, I'll request review from you.

lambdas/client-transform-filter-lambda/src/services/validators/event-validator.ts

rhyscoxnhs · 2026-02-18T15:39:56Z

lambdas/client-transform-filter-lambda/src/services/logger.ts

+  /**
+   * Add persistent context that will be included in all subsequent log entries
+   */
+  addContext(context: LogContext): void {


Are we going to include things like COMPONENT in log context like core does?

lambdas/client-transform-filter-lambda/src/services/metrics.ts

rhyscoxnhs · 2026-02-18T15:43:53Z

lambdas/client-transform-filter-lambda/src/index.ts

+  const correlationId = extractCorrelationId(rawEvent);
+  logger.addContext({ correlationId });
+
+  logLifecycleEvent("received", {


Is it worth "received" and any other events being strongly typed? We could have it so the TypeScript typings apply to the full payload so we can't pass invalid keys for a given event.

rhyscoxnhs · 2026-02-18T15:45:40Z

lambdas/client-transform-filter-lambda/src/index.ts

+
+  // Emit metric for event received
+  await metricsService.emitEventReceived(
+    eventType ?? "unknown",


Rather than having to ?? "unknown" everywhere, it might make sense for the metrics service to do this for us by allowing the property to be undefined and filling it in with the defaults if missing.

rhyscoxnhs · 2026-02-18T15:47:16Z

lambdas/client-transform-filter-lambda/src/index.ts

+  await metricsService.emitDeliveryInitiated(clientId || "unknown");
+
+  // Clear context for next event
+  logger.clearContext();


Is there ever a chance of the handler being invoked multiple times in succession in the same NodeJS runtime? Considering we're using async / await everywhere, this could lead to the context being cleared before one request fully finishes, as different parts of the code will resume in a non-deterministic order.

We may instead want to have unique elements of the logging context always be passed around in log calls to avoid this situation.

rhyscoxnhs · 2026-02-18T15:48:28Z

lambdas/client-transform-filter-lambda/src/index.ts

+    });
+    await metricsService.emitTransformationFailure(
+      eventErrorType,
+      "TransformationError",


I think these also might benefit from strong typing

rhyscoxnhs · 2026-02-18T15:49:23Z

lambdas/client-transform-filter-lambda/jest.config.ts

-      functions: 100,
-      lines: 100,
-      statements: -10,
+      branches: 50,


Are these thresholds intentional for all work going forwards?

This reverts commit 4e15075.

This reverts commit 6287115.

…operties. Set env/namespace props in terraform

rhyscoxnhs

No real fundamental issues here. I've left some minor suggestions for improvements, but there's no need to block a merge for these.

rhyscoxnhs · 2026-02-26T15:21:22Z

infrastructure/terraform/components/callbacks/locals.tf

The comments added to this file seem fairly redundant, maybe we can strip these out?

rhyscoxnhs · 2026-02-26T15:25:56Z

infrastructure/terraform/components/callbacks/variables.tf

+variable "deploy_mock_webhook" {
+  type        = bool
+  description = "Flag to deploy mock webhook lambda for integration testing (test/dev environments only)"
+  default     = true # CCM-14200: Temporary test value, revert to false


Do we need to revert this to false now?

rhyscoxnhs · 2026-02-26T15:32:55Z

lambdas/client-transform-filter-lambda/src/services/transformers/channel-status-transformer.ts

+    notifyData.supplierStatus.toLowerCase() as ClientSupplierStatus;
+
+  const attributes: ChannelStatusAttributes = {
+    messageId: notifyData.messageId,


This can just be messageId, since it's destructured above

rhyscoxnhs · 2026-02-26T15:34:33Z

lambdas/client-transform-filter-lambda/src/services/transformers/message-status-transformer.ts

+  );
+
+  const attributes: MessageStatusAttributes = {
+    messageId: notifyData.messageId,


Same as the other one - this can just be messageId,

rhyscoxnhs · 2026-02-26T15:40:36Z

lambdas/mock-webhook-lambda/src/index.ts

+  level: process.env.LOG_LEVEL || "info",
+});
+
+function isValidCallbackPayload(payload: unknown): payload is CallbackPayload {


This might be easier to understand as a zod schema, it's a bit hard to parse as it is right now

…to use but its more complex

…ay want to use but its more complex" This reverts commit f27a558.

This reverts commit b50026e.

mjewildnhs added 14 commits February 10, 2026 16:13

First pass implementation

fd5a645

Update lambda placeholder IAM policy name

84ae5d5

Use shared module for s3 config

8f8f6af

Fix sonar scanner props

924357d

Placeholder tests to test coverage

11c6c81

Fix lambda jest config

4e27d7a

Better name for config bucket

a1fa079

Update with data-model changes

ff16952

Update event names/terminology and remove nhsnumber, routingplan fiel…

ded5a05

…ds which aren't needed

Refactor model type structure

7f340cc

Remove unncessary sonar exclusions

152ef89

update teamResponsible enum

7ca4971

Update event schema based on guidance from meeting

40f54e9

fixup! Update event schema based on guidance from meeting

ac0273c

mjewildnhs force-pushed the feature/CCM-14200-event-driven-callback-delivery branch from 5dd5607 to c8d84f7 Compare February 18, 2026 11:39

WIP - US1 tasks

55efe4e

mjewildnhs force-pushed the feature/CCM-14200-event-driven-callback-delivery branch 2 times, most recently from 9447393 to c90f89d Compare February 18, 2026 11:52

mjewildnhs changed the base branch from main to feature/CCM-14199-foundation February 18, 2026 13:20

Copilot AI mentioned this pull request Feb 18, 2026

Provide code review feedback for CCM-14200 event-driven callback delivery #34

Closed

mjewildnhs changed the base branch from feature/CCM-14199-foundation to main February 18, 2026 13:24

mjewildnhs marked this pull request as ready for review February 18, 2026 13:40

mjewildnhs requested review from a team as code owners February 18, 2026 13:40

mjewildnhs marked this pull request as draft February 18, 2026 13:41

mjewildnhs closed this Feb 18, 2026

mjewildnhs reopened this Feb 18, 2026

rhyscoxnhs reviewed Feb 18, 2026

View reviewed changes

mjewildnhs added 3 commits February 24, 2026 15:05

Refactor lambda to tidy up observablity

9c4a844

Revert "explicitly set pull-request read permission"

79919f8

This reverts commit 4e15075.

Swap todo for comment for snar

fd433af

mjewildnhs force-pushed the feature/CCM-14200-event-driven-callback-delivery branch from 84de504 to fd433af Compare February 24, 2026 15:06

Turn off client creation

6287115

mjewildnhs force-pushed the feature/CCM-14200-event-driven-callback-delivery branch from c412701 to 6287115 Compare February 24, 2026 16:49

mjewildnhs added 9 commits February 25, 2026 12:31

Revert "Turn off client creation"

bed3834

This reverts commit 6287115.

Update zod/pino and update validation test assertions

432a80c

DI for handler and more test coverage

276e475

Use mock pino in mock lambda test

1af1b86

Delete test which is no longer required as coverage provided elsewhere

a011e6d

Fix cloudwatch events - single dimension with env, other fields as pr…

72132ef

…operties. Set env/namespace props in terraform

Fix event pipe template to align with lambda output

aff9961

Remove dataschemaversion from event pipe input template and bus rule

372f8fe

Var for pipe log level

92f6f4e

rhyscoxnhs previously approved these changes Feb 26, 2026

View reviewed changes

REVIEW: Event bus logging - theres a new native approach we may want …

f27a558

…to use but its more complex

mjewildnhs dismissed rhyscoxnhs’s stale review via f27a558 February 26, 2026 16:17

mjewildnhs added 12 commits February 26, 2026 16:42

Revert "REVIEW: Event bus logging - theres a new native approach we m…

fd457d5

…ay want to use but its more complex" This reverts commit f27a558.

Remove event parameters which don't work in batch scenario

96d857c

Fix correlation ID on delivery initiated event/logging

a5d0cfe

KEEP? - attempt API destination permission fix

b50026e

DROP - Enable INFO logging on event pipe

3048fe4

Revert "KEEP? - attempt API destination permission fix"

30730ac

This reverts commit b50026e.

Permissions on lambda to allow it to be invoked without IAM

781dc47

fixup! Permissions on lambda to allow it to be invoked without IAM

738dbef

Log the received payload in the mock lambda

124d69d

fixup! Log the received payload in the mock lambda

850ba61

Fix validation and correlation id

634334c

KEEP? - attempt to fix DLQ permission

848f634

Conversation

mjewildnhs commented Feb 17, 2026

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

mjewildnhs commented Feb 18, 2026

Uh oh!

Copilot AI commented Feb 18, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhyscoxnhs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants